Skip to content

Separate Warning into its own channel, refactor transaction rejection naming#268

Merged
rustaceanrob merged 5 commits intomasterfrom
warn-1-14
Jan 15, 2025
Merged

Separate Warning into its own channel, refactor transaction rejection naming#268
rustaceanrob merged 5 commits intomasterfrom
warn-1-14

Conversation

@rustaceanrob
Copy link
Collaborator

Previous to this PR, the relationship between Log and Warning is that a Warning is contained in a Log. Because the log channel had limited capacity, a Warning event can technically be missed if the receiver is lagging behind. To compensate for this, transaction rejections at the network level were put into the Event enum, which have an unbounded channel to miss any Event. In reality though, a user would probably not want to miss any Warning at all, including transaction rejections.

As an aside, the term "failure" in FailurePayload is not correct, as a single node may reject a transaction while others accept it. Instead, the FailurePayload is renamed to RejectPayload to more accurately reflect the situation.

This PR introduces the RejectPayload into the transaction rejection variant on Warning. In addition, the Warning variant is removed from Log, and Warning events are given their own channel. Now there is a clear separation of concerns between a Log, Warning, and Event without ambiguity of what data should be in each type.

@nyonson the bulk of the refactor are one line changes to account for this new warning channel sender/receiver. I tried to break the commits into easily reviewable parts, minus the head commit that has to do some heavy lifting. Of note in that one, any time the node needs to talk to the client, it must do so through the Dialog struct. To redirect the Warning messages to the new channel instead of with Log, all that changes is the implementation of Dialog::send_warning.

A transaction that is met with a reject message may be for a low fee rate.
When connected to multiple peers, it is possible to receive a reject, but
the transaction may still propagate. In this sense, "failure" is not the
correct term
let _ = self.log_tx.send(Log::Dialog(message)).await;
}

pub(crate) async fn send_warning(&self, warning: Warning) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the send call is async right? Kinda looks weird dropping the wrapper async if that is the case. Are there any issues in the tokio runtime with this though?

Copy link
Collaborator Author

@rustaceanrob rustaceanrob Jan 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

UnboundedSender<T> is sync. CI would hopefully fail for not awaiting a future

The previous bounded mpsc channel has an async send call.

Copy link
Collaborator

@nyonson nyonson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 94dde90

Thanks for the write up and commit break down, super helpful. I need to dig into these channels more, but makes sense to me from a high level.

@rustaceanrob rustaceanrob merged commit aa06cc5 into master Jan 15, 2025
@rustaceanrob rustaceanrob deleted the warn-1-14 branch January 15, 2025 04:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants